Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(event-handler): prefixes to strip for custom mappings #579

Merged
merged 12 commits into from
Aug 19, 2021

Conversation

michaelbrewer
Copy link
Contributor

@michaelbrewer michaelbrewer commented Aug 1, 2021

Issue #, if available: aws-powertools/powertools-lambda#34

  • closes #576

Description of changes:

Changes:

  • Add prefix to be stripped from the event path, default is None which does not modify the path
  • Add docstring for serializer

Example usage:

  • Passing in multiple strip_prefixes "/unique/v1" and "/foo/v1"
  • Route /status will then match on /unique/v1/status (and /status)
  • By default strip_prefixes is None, and therefore does not touch the event path
  • Note the prefix should not end with a /
import os
from aws_lambda_powertools.event_handler.api_gateway import ApiGatewayResolver

app = ApiGatewayResolver(strip_prefixes=["/unique/v1", "/foo/v1"])

@app.get("/status")
def foo():
    return {"status": "OK"}

# All calls should return a 200
response = app({"httpMethod": "GET", "path": "/unique/v1/status"}, None)
assert response["statusCode"] == 200
response = app({"httpMethod": "GET", "path": "/foo/v1/status"}, None)
assert response["statusCode"] == 200
response = app({"httpMethod": "GET", "path": "/status"}, None)
assert response["statusCode"] == 200

Checklist

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 1, 2021
@michaelbrewer
Copy link
Contributor Author

@walmsles this might help solve the issue you raised.

@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2021

Codecov Report

Merging #579 (1d6b49d) into develop (61e7f2d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #579   +/-   ##
========================================
  Coverage    99.95%   99.95%           
========================================
  Files          114      114           
  Lines         4581     4594   +13     
  Branches       249      253    +4     
========================================
+ Hits          4579     4592   +13     
  Partials         2        2           
Impacted Files Coverage Δ
aws_lambda_powertools/event_handler/api_gateway.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61e7f2d...1d6b49d. Read the comment docs.

@walmsles
Copy link
Contributor

walmsles commented Aug 1, 2021

Hi @michaelbrewer it kind of does in a limited way but would think there should be a no code/config option of doing this to use APIGW fully.

API GW allows me to create a lambda that can be mapped to multiple domains/mappings and I should be free to do that to mount an API with resolver to multiple domains with potentially different mapping paths without needing to consider environment setup as well.

Does the solution allow for multiple mappings? It seems singular.

Let me add some more comments/data to the original request to help more. I feel a design change on the actual router implementation would be simpler, less code and more flexible (although might feel like a rewrite so needs careful consideration)

@michaelbrewer
Copy link
Contributor Author

Hi @michaelbrewer it kind of does in a limited way but would think there should be a no code/config option of doing this to use APIGW fully.

API GW allows me to create a lambda that can be mapped to multiple domains/mappings and I should be free to do that to mount an API with resolver to multiple domains with potentially different mapping paths without needing to consider environment setup as well.

Does the solution allow for multiple mappings? It seems singular.

Let me add some more comments/data to the original request to help more. I feel a design change on the actual router implementation would be simpler, less code and more flexible (although might feel like a rewrite so needs careful consideration)

Ok interesting so you have the same "status" lambda for many different api gateways / mappings and it needs to automagically find this "status" route?

With some test events maybe there is a way, but I would not to result in strange breaking behavior.

@michaelbrewer
Copy link
Contributor Author

@walmsles also note, this would have to work for Api Gateway V1, V2 and ALB lambdas.

@walmsles
Copy link
Contributor

walmsles commented Aug 1, 2021

@michaelbrewer understand - not familiar with all the event models for these other API components so maybe there is no nice way - Have added a sample test event to the original request for discussion :-)

@walmsles
Copy link
Contributor

walmsles commented Aug 1, 2021

@michaelbrewer fair on support for ALL event formats. What you have proposed is useful - the use case of same lambda to many mappings is an unlikely use case but APIGW does allow/support that.

For my exact use case this week - it was the need to having my ApiGatewayResolver configured differently to my APIGW config which felt weird and wrong :-) The prefix idea will assist and resolve this particular use case so happy if that's the outcome.

I don't see where I would actually normally use the same APIGW mapped to many custom domains - the use case exists and is where my Software engineering brain went thinking about how APIGW works in this case - but in reality is probably a weird edge case :-)

@michaelbrewer
Copy link
Contributor Author

@walmsles - i will run some test cases on a deployed lambda to see if there is a method to determine the internally mapped lambda vs the custom mapped lambda

@michaelbrewer michaelbrewer marked this pull request as draft August 1, 2021 01:55
@michaelbrewer michaelbrewer changed the title feat(event-handler): Allow for a configured common prefix feat(event-handler): Allow for a configured common prefix [MAYBE NOT THE BEST SOLUTION] Aug 1, 2021
@heitorlessa
Copy link
Contributor

I'd like us to think how we could solve this on our side as a fallback mechanism without having to ask customers to change their code - whether they use custom domain mappings or not.

@michaelbrewer
Copy link
Contributor Author

@heitorlessa - any ideas would be great, based on what we get back from the aws events.

@michaelbrewer michaelbrewer marked this pull request as ready for review August 14, 2021 06:13
@michaelbrewer michaelbrewer changed the title feat(event-handler): Allow for a configured common prefix [MAYBE NOT THE BEST SOLUTION] feat(event-handler): Allow for a configured common prefix [POSSIBLE SOLUTION] Aug 14, 2021
@heitorlessa
Copy link
Contributor

heitorlessa commented Aug 17, 2021

UPDATE: Option 3 won't work as REST API v1 also includes the stage name (see new comment)

I totally missed option 3, since that seems to be always true in REST API v1 payload: requestContext. path

Since we know upfront whether customers are using it for REST API v1, ALB, or HTTP API, what's the downside of using requestContext.path for REST API v1 only besides breaking customers unit tests?


Replaced _remove_prefix with _extract_path:

    def _resolve(self) -> ResponseBuilder:
        """Resolves the response or return the not found response"""
        method = self.current_event.http_method.upper()
        path = self._extract_path()   # NEW
        for route in self._routes:
            if method != route.method:
                continue
            match: Optional[re.Match] = route.rule.match(path)
            if match:
                logger.debug("Found a registered route. Calling function")
                return self._call_route(route, match.groupdict())  # pass fn args

        logger.debug(f"No match found for path {path} and method {method}")
        return self._not_found(method)

    # NEW
    def _extract_path(self) -> str:
        """Extracts correct path regardless of custom domain mapping setting"""
        resolver = self._proxy_type
        if self._proxy_type == ProxyEventType.APIGatewayProxyEvent:
            return self.current_event.request_context.path

@heitorlessa
Copy link
Contributor

heitorlessa commented Aug 17, 2021

UPDATE: Option 3 won't work as REST API v1 also includes the stage name (see new comment)

I totally missed option 3, since that seems to be always true in REST API v1 payload: requestContext. path

Since we know upfront whether customers are using it for REST API v1, ALB, or HTTP API, what's the downside of using requestContext.path for REST API v1 only besides breaking customers unit tests?

Replaced _remove_prefix with _extract_path:

    def _resolve(self) -> ResponseBuilder:
        """Resolves the response or return the not found response"""
        method = self.current_event.http_method.upper()
        path = self._extract_path()   # NEW
        for route in self._routes:
            if method != route.method:
                continue
            match: Optional[re.Match] = route.rule.match(path)
            if match:
                logger.debug("Found a registered route. Calling function")
                return self._call_route(route, match.groupdict())  # pass fn args

        logger.debug(f"No match found for path {path} and method {method}")
        return self._not_found(method)

    # NEW
    def _extract_path(self) -> str:
        """Extracts correct path regardless of custom domain mapping setting"""
        resolver = self._proxy_type
        if self._proxy_type == ProxyEventType.APIGatewayProxyEvent:
            return self.current_event.request_context.path

It sounded too good to be true on only relying on requestContext.path...

This problem gets further complicated when you have nested mapping paths of variable lengths. This alone will be difficult to keep track in the environment variable as you might have countless of these, if you follow this edge case of having multiple APIs with different mappings pointing to the same Lambda fn.

The only solution I can think of is:

  • Code level change. Accept a list of possible prefixes, short-circuit the first match via any(), then remove it from the path string
  • No code change. Support an environment variable that use comma-separated values of prefixes we must be aware of.

Both leak the abstraction but I can't really think of anything else that would work for all scenarios without change one's code, as recommending customers to move to HTTP API isn't a good solution (feature parity!).

what do you reckon @walmsles @michaelbrewer?

If you agree, we have logic that can be reused to accept both explicit and env var mechanisms.


Single custom domain mapping path - v1

{
    "path": "/v1/payment",
    "resource": "/payment",
    "requestContext": {
        "resource": "/payment",
        "path": "/v1/payment",
        "httpMethod": "GET",
        "requestContext": {
            "resourceId": "j9knhf",
            "resourcePath": "/payment",
            "httpMethod": "GET",
            "path": "/v1/payment",
            "stage": "default",
            "domainPrefix": "api",
            "domainName": "api.serverlessa.dev",
        },
    }
}

Nested custom domain mapping path - v1/nested

Custom domain mapping - proxy resource

{
    "path": "/v1/nested/payment/123456789-afekl-13456/",
    "resource": "/payment/{invoice+}",
    "requestContext": {
        "resource": "/payment/{invoice+}",
        "path": "/v1/nested/payment/123456789-afekl-13456/",
        "httpMethod": "GET",
        "requestContext": {
            "resourceId": "8em8dt",
            "resourcePath": "/payment/{invoice+}",
            "httpMethod": "GET",
            "path": "/v1/nested/payment/123456789-afekl-13456/",
            "stage": "default",
            "domainPrefix": "api",
            "domainName": "api.serverlessa.dev",
        }
    }
}

@heitorlessa
Copy link
Contributor

@michaelbrewer lemme know what types of events you want me to generate to help with tests. I can also help write these too.

These events above are actual events coming from API GW hence why my surprise to requestContext.path idea not being an actual solution after testing - I trimmed it down to easily focus on the necessary bits.

Your initial solution on self._prefix and such continues to work, it just needs to be a list of str. We can figure out a better name for the env var later

@michaelbrewer
Copy link
Contributor Author

@heitorlessa - i am sure with more test cases we can come up with a cleaner solutions. Test cases wise:

Rest API

This is the problematic one when mapping the routes against the path, as custom mappings or a direct mapping via Route 53 gives different outcomes.

  1. Test directly via stage
  2. Test via Route 53 mapping
  3. Test via 2 different custom mappings

@michaelbrewer
Copy link
Contributor Author

michaelbrewer commented Aug 17, 2021

Http API (V1)

About the same as the Rest API (although, i think are some differences). So similar test cases as above

  1. Test directly via stage
  2. Test via Route 53 mapping
  3. Test via 2 different custom mappings

HTTP API (V2)

Does not seem to need any changes, but a confirmation would help.

Application Load Balancer

Their are not custom mapping considerations, so this should just be a regular event.

@heitorlessa
Copy link
Contributor

I'll set them up tomorrow and dump them here

@walmsles
Copy link
Contributor

@heitorlessa and @michaelbrewer Apologies for being quiet for a while. I think the solution above is workable, I feel it is an edge case (multiple API mappings) but awesome that you have arrived at a solution - let me know if I can assist.

@heitorlessa
Copy link
Contributor

heitorlessa commented Aug 18, 2021

Dropping most of the events here except Route53 (need to set that up). One issue I noticed on HTTP API too is that the stage is also part of the rawPath so this needs changing too.

I created the same API in all three versions (REST, HTTP v1, HTTP v2 payload) using /payment and /payment/{invoice+} routes.


REST API

Direct call using stage

{
    "resource": "/payment",
    "path": "/payment/",
    "httpMethod": "GET",
	"pathParameters": null,
    "requestContext": {
        "resourcePath": "/payment",
        "path": "/default/payment/",
        "stage": "default",
        "domainPrefix": "qeo6tqt75i",
        "domainName": "qeo6tqt75i.execute-api.eu-west-1.amazonaws.com",
    }
}

Single custom mapping

{
    "path": "/v1/payment",
    "resource": "/payment",
    "httpMethod": "GET",
	"pathParameters": null,
    "requestContext": {
        "resourcePath": "/payment",
        "path": "/v1/payment",
        "stage": "default",
        "domainPrefix": "api",
        "domainName": "api.serverlessa.dev",
    },
}

Nested custom mapping

{
    "path": "/v1/nested/payment/123456789-afekl-13456/",
    "resource": "/payment/{invoice+}",
    "httpMethod": "GET",
    "pathParameters": {
      "invoice": "123456789-afekl-13456"
    },
    "requestContext": {
        "resourcePath": "/payment/{invoice+}",
        "path": "/v1/nested/payment/123456789-afekl-13456/",
        "stage": "default",
        "domainName": "api.serverlessa.dev",
    }
}

Route 53

{
    "path": "/v1/payment",
    "resource": "/payment",
    "httpMethod": "GET",
	"pathParameters": null,
    "headers": {
        "User-Agent": "Amazon-Route53-Health-Check-Service (ref b83f7d76-9e42-43fb-be1d-67619818499a; report http://amzn.to/1vsZADi)"
    },
    "requestContext": {
        "resourcePath": "/payment",
        "path": "/v1/payment",
        "stage": "default",
        "domainName": "api.serverlessa.dev"
    }
}

HTTP API v1

Direct call using stage

{
    "resource": "/payment",
    "path": "/default/payment",
    "httpMethod": "GET",
	"pathParameters": null,
    "requestContext": {
        "resourcePath": "/payment",
        "path": "/default/payment",
        "stage": "default",
        "domainPrefix": "yx3odp7ks2",
        "domainName": "yx3odp7ks2.execute-api.eu-west-1.amazonaws.com"
    }
}

Single custom mapping

{
    "resource": "/payment/{invoice+}",
    "path": "/v2/payment/",
    "httpMethod": "GET",
	"pathParameters": null,
    "requestContext": {
        "resourcePath": "/payment/{invoice+}",
        "path": "/default/payment/",
        "stage": "default",
        "domainName": "api.serverlessa.dev",
        "domainPrefix": "api"
    }
}

Nested custom mapping

{
    "resource": "/payment/{invoice+}",
    "path": "/v2/payment/123456789-afekl-13456",
    "httpMethod": "GET",
    "pathParameters": {
      "invoice": "123456789-afekl-13456"
    },
    "requestContext": {
        "resourcePath": "/payment/{invoice+}",
        "path": "/default/payment/123456789-afekl-13456",
        "stage": "default",
        "domainName": "api.serverlessa.dev",
        "domainPrefix": "api"
    }
}

Route53

{
    "resource": "/payment",
    "path": "/v2/payment",
    "httpMethod": "GET",
	"pathParameters": null,
    "headers": {
        "User-Agent": "Amazon-Route53-Health-Check-Service (ref b83f7d76-9e42-43fb-be1d-67619818499a; report http://amzn.to/1vsZADi)",
    },
    "queryStringParameters": null,
    "multiValueQueryStringParameters": null,
    "requestContext": {
        "resourcePath": "/payment",
        "path": "/default/payment",
        "stage": "default",
        "domainName": "api.serverlessa.dev",
        "httpMethod": "GET"
    }
}

HTTP API v2

Direct call using stage

{
    "routeKey": "ANY /payment",
    "rawPath": "/default/payment",
    "requestContext": {
        "domainName": "swgeupeic9.execute-api.eu-west-1.amazonaws.com",
        "http": {
            "method": "GET",
            "path": "/default/payment"
        },
        "routeKey": "ANY /payment",
        "stage": "default"
    }
}

Single custom mapping

{
    "routeKey": "ANY /payment",
    "rawPath": "/default/payment",
    "requestContext": {
        "domainName": "api.serverlessa.dev",
        "http": {
            "method": "GET",
            "path": "/default/payment"
        },
        "routeKey": "ANY /payment",
        "stage": "default"
    }
}

Nested custom mapping

{
    "routeKey": "ANY /payment/{invoice+}",
    "rawPath": "/default/payment/123456789-afekl-13456",
    "pathParameters": {
      "invoice": "123456789-afekl-13456"
    },
    "requestContext": {
        "domainName": "api.serverlessa.dev",
        "http": {
            "method": "GET",
            "path": "/default/payment/123456789-afekl-13456"
        },
        "routeKey": "ANY /payment/{invoice+}",
        "stage": "default"
    }
}

Route53

{
    "routeKey": "ANY /payment",
    "rawPath": "/default/payment",
    "headers": {
        "user-agent": "Amazon-Route53-Health-Check-Service (ref b83f7d76-9e42-43fb-be1d-67619818499a; report http://amzn.to/1vsZADi)"
    },
    "requestContext": {
        "domainName": "api.serverlessa.dev",
        "http": {
            "method": "GET",
            "path": "/default/payment"
        },
        "routeKey": "ANY /payment",
        "stage": "default"
    }
}

@heitorlessa
Copy link
Contributor

Just added Route 53 event to complete the list of scenarios @michaelbrewer lemme know if there's any other data missing

@michaelbrewer michaelbrewer changed the title feat(event-handler): Allow for a configured common prefix [POSSIBLE SOLUTION] feat(event-handler): Allow for a configured prefixes to strip for custom mappings Aug 18, 2021
@michaelbrewer
Copy link
Contributor Author

@heitorlessa - ok i have added support for using a list of prefixes. also it will auto add the last /.

ie: path /fooBar/status with strip prefix /foo should not match. but /fooBar would.

@heitorlessa
Copy link
Contributor

Great, makes sense. Could you update the PR description using a list of strings instead of the env var?

We also don't know the env var values based on the description

@heitorlessa
Copy link
Contributor

LGTM - @walmsles are you happy with it? Or do we must support environment variable too?

If we do need an environment variable, we would need the values to be as comma-separated values: "/v1, /v1/payment, /v1/order"

If not, then we can merge it and move to a few last improvements and prepare a release

@walmsles
Copy link
Contributor

@heitorlessa I think no env var support is good - means AppConfig or Parameters can be used to dynamically control the prefix configuration which IMO is cleaner and what I would look to do to achieve the outcome from this feature. Working out complex Env Var values on LAMBDA is hard!

@heitorlessa
Copy link
Contributor

@heitorlessa I think no env var support is good - means AppConfig or Parameters can be used to dynamically control the prefix configuration which IMO is cleaner and what I would look to do to achieve the outcome from this feature. Working out complex Env Var values on LAMBDA is hard!

Amazing - merging it then ;) Thanks for the help flagging it @walmsles, and obviously a huge help as always to @michaelbrewer

@heitorlessa heitorlessa added feature New feature or functionality need-documentation PRs that are missing documentation labels Aug 19, 2021
@heitorlessa heitorlessa added this to the 1.20.0 milestone Aug 19, 2021
@heitorlessa heitorlessa self-assigned this Aug 19, 2021
@heitorlessa heitorlessa changed the title feat(event-handler): Allow for a configured prefixes to strip for custom mappings feat(event-handler): prefixes to strip for custom mappings Aug 19, 2021
@heitorlessa heitorlessa merged commit 3fa6803 into aws-powertools:develop Aug 19, 2021
@walmsles
Copy link
Contributor

Love your work @heitorlessa @michaelbrewer - thankyou!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or functionality need-documentation PRs that are missing documentation size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants